🎉

before	after

@donmccurdy Unrelated: Any idea why the model has .normalScale.x = - 1? It is more common to have to flip .normalScale.y, but flipping y does not appear to be required here.

From GLTFLoader.js:1992 —

// Normal map textures use OpenGL conventions:
// https://github.com/KhronosGroup/glTF/tree/master/specification/2.0#materialnormaltexture
if ( material.normalScale ) {

	material.normalScale.x = - material.normalScale.x;

}

EDIT: Related to #11315.

Wohoo! Thanks for making the PR @WestLangley and improving the solution to be more precise.

@donmccurdy I do not think GLTFLoader should be doing this.

// Normal map textures use OpenGL conventions:
// https://github.com/KhronosGroup/glTF/tree/master/specification/2.0#materialnormaltexture
if ( material.normalScale ) {

	material.normalScale.x = - material.normalScale.x;

}

A reasonable guess is the model was created for BabylonJS. If a user wants to use the model in three.js, the user should set normalScale.x = - 1.

Thanks!

A reasonable guess is the model was created for BabylonJS. If a user wants to use the model in three.js, the user should set normalScale.x = - 1.

Uh oh...

A reasonable guess is the model was created for BabylonJS.

I don't think that's the case, but would be more confident if I knew the answer to #11315. 😅

If the model uses BabylonJS's normal handedness, it is an invalid glTF file. I'll look into this in a bit.

On closer look, BabylonJS inverts normal map X when using their default left-handed system.

@donmccurdy After this PR the boombox normalmap seems to have been inverted. That material.normalScale.x = - material.normalScale.x; line may have been a hack to work around the problem this PR solved.

I've tested with NormalTangentTest and NormalTangentMirrorTest and, after this PR and with this code they look correct:

if ( material.normalScale ) {

	material.normalScale.y = - material.normalScale.y;

}

🤔

This PR is great. The gun in webgl_materials_standard was also wrong.

Before:

After:

This is all good news, but the loader should not change the model parameters. The correct fix is to properly set normalScale in the model -- or alternatively, reset it in the loader callback.

Agreed. webgl_materials_standard does not use GLTFLoader by the way, it's a obj model.

This is all good news, but the loader should not change the model parameters. The correct fix is to properly set normalScale in the model ...

The handedness of the three.js normal map system remains unanswered in #11315. If it is not the same as the OpenGL convention (which all glTF assets must use) then inverting an axis of normalScale in GLTFLoader to convert seems reasonable, yes? BabylonJS does precisely that, because they're aware their system does not match OpenGL or glTF convention.

A reasonable guess is the model was created for BabylonJS. If a user wants to use the model in three.js, the user should set normalScale.x = - 1.

Actually, this shouldn't be the problem. Otherwise this would be a bug in the GLTF spec. The model should look identical in any engine.

I would suggest standardizing on Unity and UE4 style normal map interpretation. I believe they are the same thing.

What is three.js's normal map handedness convention?

I would say the three.js tangent space coordinate convention is positive-x is to the right, and positive-y is up.

Hilarious, UE4 and Unity have different conventions - in fact the supporters of each of the two formats have equal supporters:

http://www.werwackfx.com/index.php/graphictools/graphicsarticles/graphics-articles/45-normalmapsart

I think you should then have a parameter to support both, call one OpenGL and the other DirectX, and have documentation that lists the various programs that support these two styles. OpenGL might as well be the default.

I think you should then have a parameter to support both

We have a parameter that supports both: .normalScale. It's purpose is to support normal maps authored for other engines. Of course, it also serves to reduce, or increase, the normal map's effect.

My recommendation is a public parameter called normalMapStyle. It would be separate from normalMapScale, which technically should be a scalar anyhow.

Thus you would have one style parameter and one scale parameter. It can still be converted into a Vec3 when passed to the shader as it is right now, but my suggested parameterization is a more usable API because it matches how developers think about the problem, rather than expecting developers to jump through non-obvious mental hoops in order to figure out ThreeJS - see @donmccurdy comment. Don is smart, but ThreeJS is confusing him here.

Another option would be to document the issue whenever we figure out exactly what is the right normalScale configuration to use on each case.

Well, I just try to follow other engines, like Unity and UE4, which expose a single scalar.

The behavior when normalScale = Vec2(1,1), ThreeJS operates with brighter green being up, which is OpenGL style.

I can understand this:

material.normalStyle = THREE.DirectXNormalStyle;
material.normalScale = 0.8;

Much easier than this:

// material.normalScale = new THREE.Vec2( -0.8, 0.8 ); -- oops, got it wrong, should have flipped G, but R.
material.normalScale = new THREE.Vec2( 0.8, -0.8 );

And it also prevents people from doing just incorrect things like this:

material.normalScale = new THREE.Vec2( -1.0, 0.5 );

normalMapScale, which technically should be a scalar anyhow.

Agreed, normalScale really should be a scalar.

I think the only reason it is not a scalar is to accommodate normal maps authored assuming a different tangent-space coordinate convention.

material.normalScale.set( 0.5, - 0.5 );

Quick patch in #13784 for r92.

Do we want to try the normalScale / normalStyle change for r93, or just document this?

I suggest retaining the current pattern, as it is flexible.

material.normalScale.set( 0.5, - 0.5 );